Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest improvements #1

Merged
merged 19 commits into from
May 16, 2024
Merged

Suggest improvements #1

merged 19 commits into from
May 16, 2024

Conversation

saefstroem
Copy link
Owner

No description provided.

- chain struct that allows specification of custom optional rpc url
- setup logging
- setup sled
This commit adds:
- Chainlink aggregator ABI
- Async price updater
- Contract wrapper for chainlink contracts
- Tests
src/lib.rs Outdated

// Try to initialize and catch error silently if already initialized
// during tests this make this function throw error
if log4rs::init_config(config).is_err() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it is a good idea to initialize a logger from within a library... like tests should perhaps create their own logger? Like I would hate if this somehow interferes with my env when I load this lib.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this like totally freaks me out. + .gitignore won't help here as logs will be created inside the folder of an application using this library. I suggest either removing this completely or making logging optional via some configuration flag. A local database or configuration files is understandable, but a library/crate creating its own logs without any external control over it is a rather bad etiquette.

I would perhaps suggest accepting Option<&Path> that contains a log file name or if None, logging is skipped across the entire library.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I had a custom logging mechanism before but was told that this is bad practice and that RUST_LOG env should be used.

pub price: f64,
}

impl Serializable for PriceData {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serializable trait and impl look useless here as you can invoke bincode::serialize() directly on PriceData.


/// The latest price received for this symbol.
/// This data is directly retrieved from the underlying contract.
#[derive(Deserialize, Serialize)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding Debug to derive so that you can use println!("{:#?}", price_data); in tests to get formatted output.

let address = &contract_configuration.1;
match fetch_price_for_contract(crypto_prices.clone().configuration, address).await {
Ok(price_data) => {
match crypto_prices
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this behavior is intended, then it is cleaner to use:

if let Err(err) = crypto_prices.tree.insert( ... ) {
   log::error!("...");
}


/// Retrieves the price of an underlying asset from a particular contract
async fn fetch_price_for_contract(
rustlink_configuration: Configuration,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps passing by reference here could be more efficient?

for contract_configuration in contracts {
let symbol = &contract_configuration.0;
let address = &contract_configuration.1;
match fetch_price_for_contract(crypto_prices.clone().configuration, address).await {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass configuration by reference, you might not need to clone? (AFAICS)

@@ -0,0 +1,166 @@
mod fetcher;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't really like to create implementations in lib.rs, but there are no written rules about this. Just if the project grows it can become a bit cluttered. I would consider creating config.rs and error.rs containing appropriate structs.

Also, I would say that it is more common for crates to export Error as opposed to DatabaseError as any importer can import use xxx::Error as DatabaseError.

match crypto_prices
.tree
.insert(symbol, price_data.to_bin().unwrap())
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could perhaps use more idiomatic rust here:

fetch_price_for_contract(crypto_prices.clone().configuration, address)
  .await
  .and_then(|data| crypto_prices.tree.insert(...))
  .or_else(|err| err);

... or along these lines ...

src/lib.rs Outdated
pub fn new(
rpc_url: &str,
fetch_interval_seconds: u64,
sled_path: &str,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paths are typically handled via std::path::Path or a generic can also be useful here such as:

pub fn new<P : AsRef<Path>>( ..., sled_path: P, ...) {
   let db = sled::open(sled_path.as_ref()).unwrap();
   ...
}

src/lib.rs Outdated
fetch_interval_seconds: u64,
sled_path: &str,
contracts: Vec<(String, String)>,
) -> CryptoPrices {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change CryptoPrices to Self

Perhaps it would make sense to change new() -> Self to try_new() -> Result<Self> and handle sled opening as let db = sled::open(sled_path)?; to cascade the error to the invocator?

saefstroem and others added 5 commits May 14, 2024 01:53
Todo: deprecate logging, DB and introduce channels
Co-Authored-By: aspect <[email protected]>
- add reflector enum that allows specification on how to receive round data

Todo: deprecate global logging configuration in favor of custom logging functionality
- Added WASM compatibility and WASM build scripts
- Added graceful shutdown
- Deprecated global logging configuration
@saefstroem
Copy link
Owner Author

@aspect Relevant changes made, a review is appreciated.

@saefstroem saefstroem closed this May 16, 2024
@saefstroem saefstroem reopened this May 16, 2024
@saefstroem saefstroem merged commit 14a2115 into suggest-changes May 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants